Conversation
feiniaofeiafei
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 14, 2026 01:44
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
run buildall |
morrySnow
changed the title
Contributor
TPC-H: Total hot run time: 16668 ms |
Contributor
TPC-DS: Total hot run time: 82069 ms |
Contributor
ClickBench: Total hot run time: 14.7 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Member
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes: three P1 predicate-inference correctness gaps remain. Two can turn a successful query into an error or remove valid join rows; the third is a head-only weakening that admits rows from a contradictory filter.
Review status: capped/incomplete. The third and final permitted convergence round produced the head-only graph witness in the third comment. All currently known candidates were independently adjudicated and are included or dismissed, but the contract does not permit another round to establish no-new-finding convergence.
Checkpoint conclusions:
- Goal/correctness: the patch directionally fixes the reported DATE/signed-zero replacement and QUALIFY retention cases, but it does not fully preserve predicate semantics because of the three inline blockers.
- Scope/focus: the implementation changes are small and focused on the two inference helpers, with matching unit/regression updates; no user-supplied focus points were present.
- Concurrency: not applicable. These are per-plan FE rewrite data structures with no new shared state, threads, locks, or atomics.
- Lifecycle/static initialization: not applicable; no resource ownership, teardown, or static/global initialization changed.
- Configuration: no configuration items were added or changed.
- Compatibility: no public symbols, persisted formats, RPC/thrift contracts, or rolling-upgrade behavior changed.
- Parallel paths: traced
PullUpPredicates/InferPredicatesthrough sharedPredicateInferUtilscast extraction into both replacement and inequality inference. The shared non-injective temporal pair must be fenced at extraction, while the unrelated set-operation movement concern found during review predates and bypasses this diff. - Special conditions: the new type allowlist is explained but omits the
NoneMovableFunctioncontract; the direct-comparison exception assumes injective equality extraction; and the retained-relation update interacts unsafely with the existing bidirectional EQ clear. - Test coverage: the added unit tests and production-path regressions cover the reported DATE, signed-zero, strict/non-strict QUALIFY, and order cases. Missing negative coverage is an
assert_truejoin, a lossy TIMESTAMPTZ join, and the overlapping EQ/GT/GTE five-edge filter/permutations from the comments. - Test results: the changed golden outputs are consistent with their SQL on static inspection. This review did not run builds or tests because the review contract prohibited them. The author reports an FE build, 65 unit cases, and six regression suites; current GitHub checks show FE UT, P0 Regression, COMPILE, and CheckStyle passing, while
check_coverage_feis failing. - Observability: no increased logging or metrics are needed for these deterministic planner rewrites.
- Transactions/persistence: not applicable; there are no EditLog, failover, or persisted-state changes.
- Data writes: not applicable; the patch changes optimizer predicates only, not mutation atomicity or storage paths.
- FE/BE propagation: no new variable or protocol value crosses FE/BE boundaries.
- Performance: the new type/shape checks are constant-time per candidate and the relation assignment does not materially change the existing graph complexity; no distinct performance blocker was found.
- Other issues: every candidate is resolved in the ledger. A real pre-existing set-operation
NoneMovableFunctionmovement path and an earlier pre-existing three-edge graph witness were excluded because neither is attributable to a changed line.
Review method: static-only full-diff/control-flow review, existing-thread and raw-comment duplicate fence, three subagent rounds, and an explicit final changed-file/candidate sweep.
| keep[i] = true; | ||
| set(deduced, left, right, Relation.EQ); | ||
| // Preserve the relation of the retained predicate; an inequality is not an equality. | ||
| set(deduced, left, right, type); |
Contributor
There was a problem hiding this comment.
[P1] Preserve strict edges after recording the retained relation
This assignment creates a head-only weakening when equality and strict/non-strict edges overlap. For the ordered filter a = c AND c > a AND c >= b AND b > c AND b = c, selection starts with a=b EQ, a=c EQ, and b>a GT. Here c>=b enters this branch. Recording it as GTE makes the closure treat the later b>c input as redundant; generation then emits a=b and clear(..., EQ) also removes the distinct reverse b>a GT. The result keeps only equalities plus c>=b, so a=b=c passes although the input is contradictory. With the base revision's EQ write, b>c is retained and the contradiction remains. Please preserve simultaneous reverse relations (or make EQ clearing relation-aware) and add this ordered case plus permutations to the semantic oracle and a filter regression.
Contributor
There was a problem hiding this comment.
Requesting changes (summary-only): the final live duplicate recheck found that exact-head review 5206987890 already raised every distinct blocking issue, so this review intentionally adds no duplicate inline comments. I independently reproduced the two result-correctness roots at comments 4013186177 and 4013186195 and found no additional valuable finding after two convergence rounds.
Checkpoint conclusions:
- Goal/correctness: the patch fixes the reported consistent QUALIFY chain and tested DATE/signed-zero substitution cases, but it does not fully preserve query semantics. The existing exact-head threads show that a contradictory EQ/strict relation graph can become satisfiable and that a non-injective TIMESTAMPTZ-to-DATETIME equality can infer a false comparison/IN predicate.
- Scope/focus: the production changes are small and focused on the two inference helpers, with matching unit and regression coverage. No additional user-provided focus was present.
- Concurrency: not applicable. These are per-plan FE rewrite structures with no new shared state, threads, locks, or atomics.
- Lifecycle/static initialization: not applicable; no ownership, teardown, static, or global initialization changed.
- Configuration: no configuration item was added or changed.
- Compatibility: no public symbol, persisted format, RPC/thrift contract, or rolling-upgrade behavior changed.
- Parallel paths and rule staging: traced replacement followed by inequality inference through filters, joins (including mark/outer/semi/anti restrictions), window/QUALIFY, set operations, and both repeated
InferPredicates/pushdown stages. Those paths make both blocking predicates executable; no separate registration, property, or rewrite-loop issue was found. - Special conditions: the new direct-predicate exception requires an injective extracted equality, and the retained-relation update still interacts with one-relation-per-direction storage and symmetric EQ clearing. The existing inline threads give concrete plan trees and fix directions.
- Test coverage: the new deterministic unit/regression tests cover the advertised relation orders, qualifiers, DATE/DATETIME, decimal scale, signed zero, widening casts, and direct comparison/IN/NOT IN cases. They omit contradictory EQ/strict graphs and non-injective TIMESTAMPTZ casts.
- Test results: all changed golden outputs match their SQL inputs on static inspection. This review did not run builds or tests because the review contract prohibited them. The author reports an FE build, 65 unit tests, and six regression suites; current GitHub checks show COMPILE, FE UT, P0 Regression, NonConcurrent Regression, and CheckStyle passing, while
check_coverage_feis failing. - Observability: no additional logs or metrics are needed for these deterministic planner rewrites.
- Transactions/persistence and data writes: not applicable; there are no EditLog, failover, atomicity, mutation, or storage-path changes.
- FE/BE propagation: no new variable or protocol value crosses the FE/BE boundary.
- Performance: the added type/shape checks are constant-time per replacement candidate, and the relation assignment does not change the existing graph complexity; no material CPU, allocation, or memory issue was found.
- Other issues/completion: all candidates are resolved. The two substantiated findings are already covered by exact-head inline comments, the other live concern was duplicate-fenced, and the full eight-file final sweep found no third distinct issue. This review is complete and statically verified at head
1acbe050b88ea9687e4594e2d83cd9285a59b1c0.
feiniaofeiafei
added a commit
to feiniaofeiafei/doris
that referenced
this pull request
Sep 17, 2026
…erence suites ### What problem does this PR solve? Related PR: apache#67919 Problem Summary: Add a filter regression for equality and reverse strict inequalities, including reordered and commuted comparisons, nullable rows, and a non-strict positive control. Extend the QUALIFY regression with the materialized-window comparison from the original reproduction. Move both suites introduced by the related PR and their expected files into nereids_rules_p0/infer_predicate. Keep all 120 input-order permutations across three qualifier layouts focused on input predicate selection, with reported-order coverage for full inference; separate graph-construction overwrites in full inference are outside this repair. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test / Manual test - All 66 tests in UnequalPredicateInferTest and InferPredicateByReplaceTest passed using run-fe-ut.sh. - All seven suites passed: infer_predicate_qualify, infer_predicate_reverse_relation, infer_predicate_replace_type, infer_unequal_predicates, extend_infer_equal_predicate, infer_datetimev2_cast_precision, and infer_predicate. - New and updated expected files were generated by run-regression-test.sh and verified by rerunning comparisons. - DISABLE_BUILD_UI=ON ./build.sh --fe passed on dev, including Checkstyle. The default UI build is blocked by Node.js 16 lacking node:util.styleText. - Deployed the built FE library to the requested test cluster, verified the JAR checksum and clear() bytecode, and retained the previous library backup. - Final FE Checkstyle passed with zero violations. - Behavior changed: No. Test coverage and test locations only. - Does this need documentation: No.
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes (summary-only): the exact-head duplicate fence already contains the only surviving blocker, so this review intentionally adds no duplicate inline comment. Current head 89d698b217567fbb63810e8b27ace5f1bc51bd01 statically resolves existing comments 4013186177 (lossy TIMESTAMPTZ cast extraction) and 4013186189 (NoneMovableFunction propagation), but the wrong-row failure in 4013186195 remains reproducible at UnequalPredicateInfer.java:452. The one-relation-per-direction matrix can overwrite an opposing strict fact, and later clear(..., EQ) removes the reverse edge, allowing an all-equal row through a contradictory filter.
Checkpoint conclusions:
- Goal/correctness: the replacement/cast portion now preserves the reviewed type, signed-zero, TIMESTAMPTZ, and non-movable-function semantics. The inequality portion does not fully accomplish the PR goal because the existing overlapping EQ/GT/GTE witness still weakens the input predicate set.
- Scope/focus: the production changes are small and focused across the two inference helpers and date injectivity metadata, with matching unit/regression additions. There was no additional user-provided focus.
- Concurrency: not applicable. These are per-plan FE rewrite structures with no new shared state, threads, locks, or atomics.
- Lifecycle/static initialization: not applicable; no resource ownership, teardown, static initialization, or global initialization changed.
- Configuration: no configuration item was added or changed.
- Compatibility: no persisted format, public function symbol, RPC/thrift contract, or rolling-upgrade behavior changed.
- Parallel paths and staging: traced
PullUpPredicatesandInferPredicatesthrough shared cast extraction, replacement, inequality inference, filter replacement, sibling join pushdown, window/QUALIFY, aggregate-key simplification, and DISTINCT set-operation pushdown. No separate registration, property, or rewrite-loop issue was found. - Special conditions: exact-type arbitrary-expression substitution,
NoneMovableFunctionrejection, and injective DATE/DATEV2 cast peeling are justified by concrete runtime representations. The remaining unsafe condition is the interaction between retained-relation overwrite and symmetric equality clearing already documented in4013186195. - Test coverage: the new unit and regression cases cover advertised relation orders/qualifiers, DATE/DATETIME representation, signed zero, direct comparison/IN/NOT IN, non-movable assertions, and lossy TIMESTAMPTZ joins. The inequality oracle does not cover overlapping relations on the same directed pair, which is the existing blocker's required negative case.
- Test results: all 16 changed files and every new golden row were checked statically and are internally consistent. This review did not run builds or tests because the review contract prohibits them; it does not independently validate the author's reported executions.
- Observability: no new logs or metrics are needed for these deterministic planner rewrites.
- Transactions/persistence and data writes: not applicable; there are no EditLog, failover, mutation atomicity, or storage-path changes.
- FE/BE propagation: no new variable or protocol value crosses the FE/BE boundary.
- Performance: the added predicate-tree scans and type checks are bounded by existing inference inputs and do not change graph complexity; no material CPU, allocation, or memory concern was found.
- Other issues/completion: all candidates are resolved. Two normal full-review scopes plus a separate risk-focused scope returned
NO_NEW_VALUABLE_FINDINGSin Round 1; larger graph sweeps reproduced only the existing4013186195root. The review is complete, duplicate-safe, and static-only at the stated head.
Please resolve the existing line-452 P1 and add its overlapping-edge permutation/regression coverage before merge.
Contributor
TPC-H: Total hot run time: 27570 ms |
Contributor
TPC-DS: Total hot run time: 154208 ms |
Contributor
ClickBench: Total hot run time: 24.06 s |
feiniaofeiafei
added a commit
to feiniaofeiafei/doris
that referenced
this pull request
Sep 18, 2026
### What problem does this PR solve? Related PR: apache#67919 Problem Summary: Clearing a selected equality also removes the reverse graph entry even when that entry holds an independent inequality. For the ordered filter a = c AND c > a AND c >= b AND b > c AND b = c, inference can first discard strict input predicates as redundant and then erase their remaining strict constraint while emitting an equality. Equal-valued rows consequently pass a contradictory filter. Clear the reverse entry only when it also represents equality. Add semantic checks for the reported order and all 120 permutations across three qualifier layouts, covering both predicate inference entry points. ### Release note Fix incorrect rows returned when predicate inference removes reverse inequalities while emitting equalities. ### Check List (For Author) - Test: Unit Test / Manual test - The new semantic test reproduces the failure before the fix: 26 tests run, one failure, with a = b = c = 0 incorrectly passing. - FE Checkstyle passed with zero violations. - Post-fix unit tests and cluster regression validation will follow on the dev build host. - Behavior changed: Yes. Reverse inequality constraints are retained when clearing an equality. - Does this need documentation: No.
feiniaofeiafei
added a commit
to feiniaofeiafei/doris
that referenced
this pull request
Sep 18, 2026
### What problem does this PR solve? Related PR: apache#67919 Problem Summary: Add a filter regression for a = c AND c > a AND c >= b AND b > c AND b = c. Include reversed predicate order, commuted comparisons, null inputs, and a satisfiable non-strict control to ensure inference preserves contradictions without discarding valid equal-valued rows. ### Release note None ### Check List (For Author) - Test: Regression validation and runner-generated expectations pending on dev - Behavior changed: No, tests only - Does this need documentation: No
feiniaofeiafei
added a commit
to feiniaofeiafei/doris
that referenced
this pull request
Sep 18, 2026
### What problem does this PR solve? Related PR: apache#67919 Problem Summary: Record the regression-runner output for contradictory filters containing overlapping equalities and strict reverse relations. Original, reversed, and commuted forms must return no rows, while the non-strict control keeps both equal non-null rows. Before the fix, the original and commuted forms each incorrectly returned two rows. ### Release note None ### Check List (For Author) - Test: Regression test: generated the new .out with run-regression-test.sh and reran all nine related suites successfully. FE build and Checkstyle passed. Manual before/after verification confirmed the incorrect rows are removed. - Behavior changed: No, test expectations only - Does this need documentation: No
feiniaofeiafei
added a commit
to feiniaofeiafei/doris
that referenced
this pull request
Sep 18, 2026
### What problem does this PR solve? Related PR: apache#67919 Problem Summary: inferUnequalPredicates replaces predicates and must preserve full filter equivalence across all 120 input orders. inferAllPredicates supplies implied facts for redundant-predicate suppression rather than replacing the original filter; it need not return an equivalent contradictory filter for every order. Keep the reported-order coverage for both entry points and the full permutation oracle for replacement inference. ### Release note None ### Check List (For Author) - Test: The initial FE unit run identified the overly broad inferAllPredicates assertion. Replacement inference passed all tested permutations; full rerun pending. Nine regression suites passed. - Behavior changed: No, test assertion scope only - Does this need documentation: No
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
feiniaofeiafei
force-pushed
the
codex/fix-predicate-inference-semantics
branch
from
September 18, 2026 02:33
f2fa306 to
d0fef8d
Compare
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
### What problem does this PR solve? Related PR: apache#67919 Problem Summary: Record the regression-runner output for contradictory filters containing overlapping equalities and strict reverse relations. Original, reversed, and commuted forms must return no rows, while the non-strict control keeps both equal non-null rows. Before the fix, the original and commuted forms each incorrectly returned two rows. ### Release note None ### Check List (For Author) - Test: Regression test: generated the new .out with run-regression-test.sh and reran all nine related suites successfully. FE build and Checkstyle passed. Manual before/after verification confirmed the incorrect rows are removed. - Behavior changed: No, test expectations only - Does this need documentation: No
feiniaofeiafei
force-pushed
the
codex/fix-predicate-inference-semantics
branch
from
September 20, 2026 08:24
6b9d127 to
84e061d
Compare
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
Contributor
There was a problem hiding this comment.
Requesting changes: four distinct P1 evaluation-domain gaps remain at exact head 84e061d80a2727f9b49eec749bfdb1b768a1addf. The new NoneMovableFunction marker is honored by the changed predicate paths, but eager aggregation and several Limit/TopN project rewrites still add or suppress evaluation of to_bitmap_with_check on rows outside its original domain.
Checkpoint conclusions:
- Goal/correctness: the replacement allowlist, injective-cast recursion, inequality relation retention, and changed movement fences are sound on static inspection. The four inline paths remain observable because invalid bitmap conversion returns
InvalidArgument. - Scope and parallel paths: all 31 authoritative changed files and the relevant inference, eager-aggregation, Limit/TopN, join, aggregate, set-operation, CTE, CASE, and project-pruning paths were covered. Existing inline threads were hard duplicate fences.
- Rule ordering/configuration: the witnesses survive the exact rewrite schedule. Forced
eager_aggregation_mode=1reaches the eager path;push_topn_to_aggis enabled by default; the Project/Limit and TopN/join transposes have no later inverse that restores the original evaluation domain. - Tests/results: the added unit and regression oracles are internally consistent for the changed inference fixes, but do not cover these four movement families. This review was static-only as required; no build or repository test was run. Current GitHub evidence shows COMPILE, CheckStyle, and several lightweight checks passing, while FE UT, regression, performance, and other jobs remain pending; those are not independent validation by this review.
- Concurrency, lifecycle, configuration changes, compatibility, persistence/transactions, data writes, FE-BE protocol propagation, and observability: not otherwise implicated by these statement-local planner changes.
- Performance: no separate material performance defect survived review; the issue is observable error semantics, not planner complexity.
- Completion: two bounded rounds completed. The final full-diff, movement-focused, and inference-focused passes all returned
NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, dismissed with evidence, or duplicate-fenced.
Contributor
TPC-H: Total hot run time: 27953 ms |
Contributor
TPC-DS: Total hot run time: 151744 ms |
Contributor
ClickBench: Total hot run time: 24.14 s |
Collaborator
Author
|
run feut |
Contributor
FE UT Coverage ReportIncrement line coverage |
yujun777
requested changes
Sep 21, 2026
yujun777
left a comment
yujun777
left a comment
Contributor
There was a problem hiding this comment.
Static review confidence: 8/10 that these are real correctness defects. They are not expected to affect every query, but each can change a successful query into an InvalidArgument error or suppress an error by changing the expression evaluation domain. The four paths below should be covered before merge.
morrySnow
approved these changes
Sep 22, 2026
morrySnow
approved these changes
Sep 23, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Problem Summary:
Fix two predicate inference errors that change query results:
chooseInputPredicatesrecords a retainedGT/GTEpredicate asEQ. The false equality can make another necessary predicate appear redundant. For example,a > b AND rn > b AND a > rncan losea > rnand return extra rows in a QUALIFY query. Record the actual relation in the working graph.The fixes are in separate commits. Shared cast extraction is unchanged, and existing regression expectations are unchanged.
Release note
Fix extra or missing rows caused by incorrect predicate inference, including QUALIFY inequality chains and substitutions inside type-sensitive or representation-sensitive expressions.
Check List (For Author)
infer_predicate_qualify,infer_predicate_replace_type,infer_unequal_predicates,extend_infer_equal_predicate,infer_predicate, andinfer_datetimev2_cast_precision. New expected files were generated by the regression runner and verified by rerunning the suites.UnequalPredicateInferTestandInferPredicateByReplaceTestpassed. New inequality checks cover 234 relation/order/qualifier combinations with 64 value assignments per combination. Substitution tests cover type/precision boundaries, signed zero, safe same-type substitutions, widening casts, and direct comparison/IN/NOT IN propagation.DISABLE_BUILD_UI=ON ./build.sh --fepassed, including Checkstyle.Check List (For Reviewer who merge this PR)